Skip to content

Allow custom coercion failure messages #1203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow custom coercion failure messages #1203

wants to merge 1 commit into from

Conversation

zuk
Copy link

@zuk zuk commented Nov 14, 2015

Currently if something goes wrong inside a custom param type's .parse method, the only feedback in the resulting validation failure response is a generic "____ is invalid" message. This pull request allows custom coercion failure messages by having the .parse method return an InvalidValue initialized with the failure message.

For example:

class Color
  attr_reader :value
  def initialize(color)
    @value = color
  end

  def self.parse(value)
    if  %w(blue red green).include?(value)
      new(value)
    else
      Grape::Validations::Types::InvalidValue.new "is not a valid color"
    end
  end
end

# ...

params do
  requires :color, type: Color, default: Color.new('blue')
end

get '/stuff' do
  # params[:color] is already a Color.
  params[:color].value
end

zuk pushed a commit to zuk/grape that referenced this pull request Nov 14, 2015
add pull request ruby-grape#1203 to CHANGELOG
760eb9e
@dblock
Copy link
Member

dblock commented Nov 14, 2015

Great, can you squash these commits please?

@zuk zuk force-pushed the custom-coercion-failure-message branch from 756e9ff to fa6b90d Compare November 15, 2015 00:46
@zuk
Copy link
Author

zuk commented Nov 15, 2015

Ok done. Not sure what's up with those failed Rubinius builds in Travis.

dblock
dblock reviewed Nov 15, 2015
View reviewed changes
lib/grape/validations/validators/coerce.rb Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in love with this. We're now explicitly checking what kind of exception we got. Any better suggestions?

@zuk
Copy link
Author

zuk commented Nov 15, 2015

I think there's a bigger problem here. Coercion and validation take separate paths but are really converging towards the same thing (i.e. we'll eventually end up needing full-fledged validation inside .parse). But this feels too monumental for a casual contributor to take on. Another way to do this would be to give custom types an optional .validate method that behaves like a regular custom validator, and when present, gets called prior to coercion. I considered doing it this way but thought the current proposal would be more likely to get pulled since it leverages the exiting BadValue type (which already seems a bit out of place... its acts like an exception, but isn't...)

@dblock dblock mentioned this pull request Nov 24, 2015
Closed
@zuk zuk force-pushed the custom-coercion-failure-message branch from fa6b90d to 242ecea Compare July 2, 2016 18:06
allow custom param coercion failure messages
ce9f494 
Custom param types can now report why coercion failed in their .parse
method by returning an InvalidValue initialized with an error message.
For example:

  Grape::Validations::Types::InvalidValue.new "is too short"
@zuk zuk force-pushed the custom-coercion-failure-message branch from 242ecea to ce9f494 Compare July 2, 2016 18:06
@dblock
Copy link
Member

dblock commented Aug 12, 2017

I encountered something related in #1675. I think our validation + coercion needs some serious revisiting. Just thinking out loud.

@zuk
Copy link
Author

zuk commented Aug 15, 2017

I agree :) We've been using our own fork of Grape with the above change, periodically rebasing to origin. It works, but it's pretty hacky. There's probably a bigger architectural issue starting to surface here, but like I said before, it feels like too much for me to try to take on.

@zuk zuk closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zuk @dblock